ARROW-6317: [JS] Implement IPC message format alignment changes by trxcllnt · Pull Request #5225 · apache/arrow

trxcllnt · 2019-08-29T05:31:18Z

Implements the IPC message format alignment changes for ARROW-6313. In this PR the MessageReader can read messages with the old alignment, but RecordBatchWriter always produces messages with the new alignment. I can add a flag if others think it'd be useful to produce messages in the old format.

trxcllnt · 2019-08-29T17:53:08Z

@wesm C++ <-> JS integration tests are passing:

emkornfield · 2019-08-31T04:42:21Z

@TheNeuralBit do you have time to review?

TheNeuralBit

LGTM, just some minor comments. Thanks Paul!

TheNeuralBit · 2019-08-31T17:40:09Z

Should we have some kind of assertion somewhere that verifies length > 0? What happens if a corrupt file has that value?

That’s effectively handled in the subsequent call. We use the length read here as the number of bytes to consume from the stream, which should be the flatbuffer metadata.

If the length is incorrect, the flatbuffer metadata created from the buffer will be empty. If the length is too long, the stream will read as many bytes as it can, and close automatically. Both cases are interpreted by the stream reader as bad input, and ignored.

TheNeuralBit · 2019-08-31T17:45:33Z

It's too bad we have to duplicate this code. Could the readers be combined somehow? Not in this PR, just curious

Unfortunately not. Async functions are implemented via promises, which don’t allow parameterizing the scheduler. Async functions force the caller to handle the returned promise, guaranteeing the function doesn’t finish its work synchronously, even if it could.

This is a key difference between JS promises and Python’s asyncio module, which does execute synchronously if no actual async work is performed.

trxcllnt · 2019-09-04T18:17:36Z

@TheNeuralBit just pushed up some changes to add a writeLegacyIpcFormat flag to the RecordBatchStreamWriter options and clarify the comment in the reader.

fsaintjacques · 2019-09-09T14:44:59Z

Is this ready for merging?

trxcllnt · 2019-09-09T14:47:34Z

@fsaintjacques I believe so yes

fsaintjacques · 2019-09-09T15:11:36Z

According to the mailing list it seems that we need to change the EOS marker size too. I'll wait for this to settle first.

wesm · 2019-09-09T15:25:45Z

I'll make a Format patch today with the changes and change the C++ library, the change shouldn't be too complex

trxcllnt · 2019-09-09T15:27:57Z

I'm testing out the new EOS bytes change in JS right now, will push a commit here in a bit if it works.

wesm · 2019-09-09T15:28:46Z

Awesome, thanks!

trxcllnt · 2019-09-12T17:14:45Z

@wesm have the C++ changes for the new 8-byte eos message been pushed to the integration branch? If so, I can pull locally and validate against the JS changes.

wesm · 2019-09-12T18:30:48Z

@trxcllnt I just pushed the change to the branch, so you can rebase and test

wesm · 2019-09-12T18:32:42Z

I just rebased

wesm · 2019-09-12T18:33:51Z

It looks like only the EOS writing is probably all that needs to be changed

trxcllnt · 2019-09-12T19:39:48Z

@wesm I pulled the latest from this branch and rebuilt everything, but getting errors in the integration runner:

C++ producing, C++ consuming

==========================================================
Testing file /home/ptaylor/dev/arrow/integration/data/struct_example.json
==========================================================
-- Creating binary inputs
-- Validating file
-- Validating stream
Segmentation fault
Traceback (most recent call last):
  File "integration/integration_test.py", line 1284, in _compare_implementations
    run_binaries(producer, consumer, test_case)
  File "integration/integration_test.py", line 1315, in _produce_consume
    consumer.stream_to_file(producer_stream_path, consumer_file_path)
  File "integration/integration_test.py", line 1554, in stream_to_file
    self.run_shell_command(cmd)
  File "integration/integration_test.py", line 1389, in run_shell_command
    subprocess.check_call(cmd, shell=True)
  File "/usr/lib/python3.6/subprocess.py", line 311, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command 'cat /tmp/tmpbp24i1lj/4ee47ec3_struct_example.producer_file_as_stream | /home/ptaylor/dev/arrow/cpp/build/debug/arrow-stream-to-file > /tmp/tmpbp24i1lj/4ee47ec3_struct_example.consumer_stream_as_file' returned non-zero exit status 139.

JS producing, C++ consuming

==========================================================
Testing file /home/ptaylor/dev/arrow/integration/data/struct_example.json
==========================================================
-- Creating binary inputs
-- Validating file
-- Validating stream
Segmentation fault
Traceback (most recent call last):
  File "integration/integration_test.py", line 1284, in _compare_implementations
    run_binaries(producer, consumer, test_case)
  File "integration/integration_test.py", line 1315, in _produce_consume
    consumer.stream_to_file(producer_stream_path, consumer_file_path)
  File "integration/integration_test.py", line 1554, in stream_to_file
    self.run_shell_command(cmd)
  File "integration/integration_test.py", line 1389, in run_shell_command
    subprocess.check_call(cmd, shell=True)
  File "/usr/lib/python3.6/subprocess.py", line 311, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command 'cat /tmp/tmpbp24i1lj/5f87e545_struct_example.producer_file_as_stream | /home/ptaylor/dev/arrow/cpp/build/debug/arrow-stream-to-file > /tmp/tmpbp24i1lj/5f87e545_struct_example.consumer_stream_as_file' returned non-zero exit status 139.

trxcllnt · 2019-09-12T20:10:01Z

@wesm nevermind, looks like it was a bad rebase. I just re-branched from ARROW-6313-flatbuffer-alignment and copied over only the JS changes and the integration tests are passing locally.

wesm

+1 Awesome, thanks.

Implements the IPC message format alignment changes for [ARROW-6313](https://issues.apache.org/jira/browse/ARROW-6313). In this PR the `MessageReader` can read messages with the old alignment, but `RecordBatchWriter` always produces messages with the new alignment. I can add a flag if others think it'd be useful to produce messages in the old format. Closes #5225 from trxcllnt/ARROW-6314 and squashes the following commits: b69bc0e <ptaylor> update ipc reader and writer for ARROW-6313 Authored-by: ptaylor <paul.e.taylor@me.com> Signed-off-by: Wes McKinney <wesm+git@apache.org>

Implements the IPC message format alignment changes for [ARROW-6313](https://issues.apache.org/jira/browse/ARROW-6313). In this PR the `MessageReader` can read messages with the old alignment, but `RecordBatchWriter` always produces messages with the new alignment. I can add a flag if others think it'd be useful to produce messages in the old format. Closes apache#5225 from trxcllnt/ARROW-6314 and squashes the following commits: b69bc0e <ptaylor> update ipc reader and writer for ARROW-6313 Authored-by: ptaylor <paul.e.taylor@me.com> Signed-off-by: Wes McKinney <wesm+git@apache.org>

trxcllnt force-pushed the ARROW-6314 branch from 6b3dbfd to 20018ad Compare August 29, 2019 06:49

TheNeuralBit approved these changes Aug 31, 2019

View reviewed changes

wesm force-pushed the ARROW-6313-flatbuffer-alignment branch from 4f9b887 to 0352456 Compare September 11, 2019 22:07

wesm force-pushed the ARROW-6314 branch from c19f082 to d58f21f Compare September 12, 2019 18:32

update ipc reader and writer for ARROW-6313

b69bc0e

trxcllnt force-pushed the ARROW-6314 branch from 19c5e0f to b69bc0e Compare September 12, 2019 20:02

wesm approved these changes Sep 12, 2019

View reviewed changes

wesm closed this Sep 12, 2019

asfimport mentioned this pull request Sep 12, 2019

[JS] Implement changes to ensure flatbuffer alignment #22697

Closed

Uh oh!

Conversation

trxcllnt commented Aug 29, 2019

Uh oh!

trxcllnt commented Aug 29, 2019

Uh oh!

emkornfield commented Aug 31, 2019

Uh oh!

TheNeuralBit left a comment

Choose a reason for hiding this comment

Uh oh!

TheNeuralBit Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

trxcllnt Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheNeuralBit Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

trxcllnt Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

trxcllnt commented Sep 4, 2019

Uh oh!

fsaintjacques commented Sep 9, 2019

Uh oh!

trxcllnt commented Sep 9, 2019

Uh oh!

fsaintjacques commented Sep 9, 2019

Uh oh!

wesm commented Sep 9, 2019

Uh oh!

trxcllnt commented Sep 9, 2019

Uh oh!

wesm commented Sep 9, 2019

Uh oh!

trxcllnt commented Sep 12, 2019

Uh oh!

wesm commented Sep 12, 2019

Uh oh!

wesm commented Sep 12, 2019

Uh oh!

wesm commented Sep 12, 2019

Uh oh!

trxcllnt commented Sep 12, 2019

Uh oh!

trxcllnt commented Sep 12, 2019

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants